Cppcheck & clang-tidy fixes #744
Conversation
|
Wow so many fixes...The branch seems to work in my testing. The |
zjeffer
force-pushed
the
fix/zjeffer/fixes
branch
2 times, most recently
from
f522f60 to
49148ce
Compare
|
In 13a1f6b, I opted to replace the SpecialNodeId namespace + Value enum with simpler |
|
Added some clang-tidy fixes in latest commits (sorry 😄) |
|
This seems like a great thing to have on our CI/CD as well! Do you plan to do that in this PR? On a side note, the openSUSE job is spewing a few compiler warnings in the build step. Can you a look at those, please? |
That would be a really great addition to our CI indeed. |
I created an issue for it but I don't plan to add it to this PR. First I wanted to fix all the errors ;) |
Fixed it. Not sure why my system and the other jobs didn't show that warning though. |
Must be some quirk/bug of GCC 10 — which only openSUSE is using. Thanks for fixing it, though. :) As for all the code changes, I didn't go through all of them, but even if I did, I wouldn't understand most of them anyway, because most C++ concepts are still voodoo to me. 😅 Anyway, I did at least test the AppImage on Linux and it seems to be working okay. I also went to test the Windows build, and got hit by a regression unrelated to this PR, but after reverting the offending commit, I verified that the Windows build also seems to be working okay. Still, I'm deferring the actual review to @nuttyartist. But if I may nitpick, I'd just squash up some of these commits for a cleaner merge. |
Please enter the commit message for your changes. Lines starting
zjeffer
force-pushed
the
fix/zjeffer/fixes
branch
from
6d722e1 to
7611aa1
Compare
There was a problem hiding this comment.
Seems to work well on my machine (macoS). @zjeffer thanks for this! Ready to merge imo, your call on whether to squash.
Command used:
All warnings are now fixed.
The command has 3 suppresions:
--suppress=missingIncludeSystem: because it can't find any Qt system libraries. Not sure how to fix this, we already pass--library=qt...--suppress=missingInclude: same reason--suppress=useStlAlgorithm: it often suggests changing a simple for-loop to a much less readable std::transform call.